Optimize redundant fail checks #118

Mingun · 2021-04-27T20:33:13Z

Port of my optimization pegjs/pegjs#400, that reduce many dumb comparisons (that is not cut by minifiers). You can see some of them on the parser.js (but enable whitespace-ignored mode on github)

I calculate possible values of the match result and do not generate conditions, that are always true or false and also didn't generate not used constants.

StoneCypher

Creating a good PR is not a simple task, yes. I don't think we should rush to merge unpolished PRs.

The difference, of course, is I'm able to say specifically what isn't acceptable, and why.

lib/compiler/passes/generate-bytecode.js

lib/compiler/passes/inference-match-result.js

Mingun

@StoneCypher, thanks for review! I'll add some comments describing the overall idea of the generate-bytecode pass, I hope, that saves your time for the next time

lib/compiler/passes/generate-bytecode.js

lib/compiler/passes/inference-match-result.js

Mingun · 2021-05-01T18:48:37Z

I've add some comments, explaining some details how the generate-bytecode pass works and fixed an error: it is impossible to remove always failed alternatives that lead to call of the custom code because it may have side effects. Added tests that proves this (run side-effects in non-matching behavior tests).

Ready for final review.

hildjj

These are all minor comments. One substantive point of discussion, though: shouldn't many of these cases be warnings (or errors if you pass in a "all warnings are errors" flag)? We don't currently have a mechanism for warnings, but if we're going to do multiple errors in #160, could we discuss also adding warnings at the same time?

CHANGELOG.md

lib/compiler/passes/generate-bytecode.js

hildjj · 2021-05-28T18:40:36Z

lib/compiler/passes/generate-bytecode.js

@@ -248,7 +269,10 @@ function generateBytecode(ast) {
    return first.concat(...args);
  }

-  function buildCondition(condCode, thenCode, elseCode) {
+  function buildCondition(match, condCode, thenCode, elseCode) {
+    if (match > 0) { return thenCode; }


If you add constants, they should be used here as well.

Mingun · 2021-05-30T18:20:51Z

Locally fixed all except constants. Will do that tomorrow.

These are all minor comments. One substantive point of discussion, though: shouldn't many of these cases be warnings (or errors if you pass in a "all warnings are errors" flag)? We don't currently have a mechanism for warnings, but if we're going to do multiple errors in #160, could we discuss also adding warnings at the same time?

Yes, always matched/never matched will report warnings when we merge warnings support.

…or result of parse, when it can be statically determined The idea in that we calculate expected rules result and use that information when generate bytecode. Some constructs of the PEG grammar never fail, for example, optional operator `?` or repetition operator `*`. So it is unnecessary to check their result for the failure. New pass `inference-match-result.js` adds to the each AST node new integer property `match`, which represents possible result of matching: - `<0` - node is never match, for example, `!('a'*)` (negation of the always matched node) - `=0` - sometimes node matched, sometimes not. This is behavior before that change - `>0` - node is always matched, for example, `'a'*` (because result is or empty array, or array with some elements)

…hangelog (review this change with the space changes ignored)

hildjj · 2021-05-31T20:09:39Z

lib/compiler/passes/generate-bytecode.js

@@ -248,7 +270,10 @@ function generateBytecode(ast) {
    return first.concat(...args);
  }

-  function buildCondition(condCode, thenCode, elseCode) {
+  function buildCondition(match, condCode, thenCode, elseCode) {
+    if (match === ALWAYS_MATCH) { return thenCode; }


This reads a lot easier. thanks for that.

hildjj · 2021-05-31T21:01:24Z

I went to add type tests for MatchResult, and found an error in the compile interface of peg.d.ts. Here's an appended patch that fixes that, and adds the comment I was talking about. Let me know your opinion, please.

…oesn't need to be an object that gets converted to an array; it can just be an array that gets copied

…er to generate MatchResult

Mingun · 2021-06-01T17:42:42Z

Ok. I've just split you commit into two and fix a small incorrectness in you understanding of a situation about possible cycle

Mingun force-pushed the inference-match-result branch from 999aba1 to 0afeb61 Compare April 27, 2021 20:52

Mingun mentioned this pull request Apr 29, 2021

Typescript, Rollup, Jest, Terser, Rimraf, announcer, de-binarying, CI platforming #107

Closed

StoneCypher suggested changes Apr 30, 2021

View reviewed changes

Mingun commented Apr 30, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

StoneCypher approved these changes Apr 30, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

Mingun force-pushed the inference-match-result branch from 0afeb61 to 913b3f2 Compare May 1, 2021 17:48

This comment has been minimized.

Sign in to view

Mingun force-pushed the inference-match-result branch from 913b3f2 to 52ed2d2 Compare May 4, 2021 18:31

hildjj added this to the 1.3 milestone May 22, 2021

Mingun force-pushed the inference-match-result branch 2 times, most recently from ccf2d2c to 19a1a50 Compare May 27, 2021 16:47

hildjj modified the milestones: 1.3, 1.2 May 28, 2021

hildjj mentioned this pull request May 28, 2021

Release 1.2 #159

Closed

3 tasks

Mingun force-pushed the inference-match-result branch from 19a1a50 to 36e9b07 Compare May 28, 2021 17:34

hildjj reviewed May 28, 2021

View reviewed changes

Mingun added 2 commits May 31, 2021 21:31

Optimization (inference match result): regenerate parser and update c…

8a4051f

…hangelog (review this change with the space changes ignored)

Mingun force-pushed the inference-match-result branch from 36e9b07 to 8a4051f Compare May 31, 2021 16:38

Mingun requested a review from hildjj May 31, 2021 16:43

hildjj reviewed May 31, 2021

View reviewed changes

hildjj added 2 commits June 1, 2021 22:38

Optimization (inference match result): Simplify compiler.passes; it d…

c82b6e9

…oesn't need to be an object that gets converted to an array; it can just be an array that gets copied

Optimization (inference match result): Test compiler interface in ord…

3a25229

…er to generate MatchResult

Mingun force-pushed the inference-match-result branch from ed1cb08 to 3a25229 Compare June 1, 2021 17:39

hildjj merged commit cb7a18b into peggyjs:main Jun 1, 2021

Mingun deleted the inference-match-result branch June 1, 2021 18:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize redundant fail checks #118

Optimize redundant fail checks #118

Mingun commented Apr 27, 2021

StoneCypher left a comment

Mingun left a comment

This comment has been minimized.

This comment has been minimized.

Mingun commented May 1, 2021

This comment has been minimized.

hildjj left a comment

hildjj May 28, 2021

Mingun May 31, 2021

Mingun commented May 30, 2021 •

edited

Loading

hildjj May 31, 2021

hildjj commented May 31, 2021

Mingun commented Jun 1, 2021

Optimize redundant fail checks #118

Optimize redundant fail checks #118

Conversation

Mingun commented Apr 27, 2021

StoneCypher left a comment

Choose a reason for hiding this comment

Mingun left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Mingun commented May 1, 2021

This comment has been minimized.

hildjj left a comment

Choose a reason for hiding this comment

hildjj May 28, 2021

Choose a reason for hiding this comment

Mingun May 31, 2021

Choose a reason for hiding this comment

Mingun commented May 30, 2021 • edited Loading

hildjj May 31, 2021

Choose a reason for hiding this comment

hildjj commented May 31, 2021

Mingun commented Jun 1, 2021

Mingun commented May 30, 2021 •

edited

Loading